Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: improve single-node.sh #4041

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Nov 20, 2024

Closes #3975

Fixes a few issues I discovered with single-node.sh

  1. single-node.sh previously created a genesis file with appVersion: 3 so even though it attempted to upgrade from v1 -> v2, that step doesn't make any sense because the node was already on v3. This PR fixes the issue by removing the --v2-upgrade-height flag from the script
  2. Add a few more logs
  3. Add a new script: single-node-upgrades.sh that can be invoked to start a single node on v1 and upgrade it: v1 -> v2 -> v3. After running the command, the user will see
An upgrade is pending to app version 3 at height 50406.

FLUPs

  1. Modify single-node.sh to override the upgrade height delay and instead upgrade from v2 -> v3 instantly: Override upgrade height delay for local devnets #4053

@rootulp rootulp self-assigned this Nov 20, 2024
@rootulp rootulp marked this pull request as ready for review November 20, 2024 19:50
@rootulp rootulp requested a review from a team as a code owner November 20, 2024 19:50
@rootulp rootulp requested review from rach-id and evan-forbes and removed request for a team November 20, 2024 19:50
@celestia-bot celestia-bot requested a review from a team November 20, 2024 19:51
Copy link
Contributor

coderabbitai bot commented Nov 20, 2024

📝 Walkthrough

Walkthrough

The changes in this pull request enhance the versioning and upgrade logic of the application. Modifications include the addition of logging statements during the upgrade process in the EndBlocker and BeginBlocker methods, ensuring strict version increments. The single-node.sh script has been updated for clarity and functionality, now starting a single node testnet on app version 3. A new script, single-node-upgrades.sh, has been introduced to facilitate the setup and upgrade process for a single node testnet.

Changes

File Change Summary
app/app.go Updated EndBlocker and BeginBlocker methods to include logging and enforce version increment logic.
scripts/single-node.sh Modified to clarify initialization for app version 3, replaced variable names, and improved output formatting.
scripts/single-node-upgrades.sh Introduced new script for setting up and upgrading a single node testnet, including functions for genesis creation and upgrade handling.

Assessment against linked issues

Objective Addressed Explanation
The application incorrectly reports its version as 3 from the start, instead of starting at version 1. The changes do not address the initial version reporting issue.
The expected upgrade path is from app version 1 to version 2 at block height 3.
There are log entries indicating an upgrade from app version 1 to 2 at block height 3.

Possibly related PRs

Suggested labels

warn:api breaking, enhancement, required

Suggested reviewers

  • evan-forbes
  • cmwaters
  • ninabarbakadze
  • staheri14

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (4)
scripts/upgrade-to-v3.sh (2)

12-16: Enhance binary verification

The binary check could be more comprehensive by also verifying the version compatibility.

Consider enhancing the verification:

 if ! [ -x "$(command -v celestia-appd)" ]
 then
     echo "celestia-appd could not be found. Please install the celestia-appd binary using 'make install' and make sure the PATH contains the directory where the binary exists. By default, go will install the binary under '~/go/bin'"
     exit 1
 fi
+# Verify minimum version requirement
+VERSION=$(celestia-appd version 2>&1)
+if ! echo "$VERSION" | grep -q "v2"; then
+    echo "Error: celestia-appd version must be v2.x.x for this upgrade"
+    exit 1
+fi

1-1: Ensure script permissions

The script will need executable permissions to run.

Run the following command after adding the script:

chmod +x scripts/upgrade-to-v3.sh
scripts/single-node.sh (2)

81-81: LGTM! Consider adding error handling

The new sed pattern correctly sets the initial app version to "1", fixing the upgrade path issue. The regex pattern is robust and will handle various formatting of the existing app_version value.

Consider adding error checking after the sed command:

-sed -i'.bak' 's/"app_version": *"[^"]*"/"app_version": "1"/' ${CELESTIA_APP_HOME}/config/genesis.json
+if ! sed -i'.bak' 's/"app_version": *"[^"]*"/"app_version": "1"/' ${CELESTIA_APP_HOME}/config/genesis.json; then
+    echo "Error: Failed to set initial app version in genesis.json"
+    exit 1
+fi

Line range hint 83-93: Consider parameterizing tracing configuration

The tracing configuration is well-structured and includes clear user feedback. However, hardcoding these values might limit flexibility for different environments.

Consider making these values configurable through environment variables:

-    trace_type="local"
+    trace_type="${CELESTIA_TRACE_TYPE:-local}"
-    trace_pull_address=":26661"
+    trace_pull_address="${CELESTIA_TRACE_PULL_ADDRESS:-:26661}"
-    trace_push_batch_size=1000
+    trace_push_batch_size="${CELESTIA_TRACE_PUSH_BATCH_SIZE:-1000}"

This would allow users to customize the tracing configuration without modifying the script.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 66699e7 and b6470c7.

📒 Files selected for processing (3)
  • app/app.go (2 hunks)
  • scripts/single-node.sh (1 hunks)
  • scripts/upgrade-to-v3.sh (1 hunks)
🔇 Additional comments (3)
scripts/single-node.sh (1)

Line range hint 108-114: Verify upgrade height configuration

The --v2-upgrade-height 3 setting needs verification to ensure it aligns with the new versioning approach where we start from app version 1.

Let's check for any related upgrade configurations:

✅ Verification successful

The upgrade height setting of 3 in the script is correct and aligns with the versioning approach

The verification shows that:

  1. The --v2-upgrade-height flag is consistently used across different scripts with appropriate values
  2. The codebase supports version-specific constants and upgrade mechanisms
  3. The setting of height 3 for local testnet is appropriate as it allows:
    • Initial blocks with version 1
    • Early upgrade testing without waiting long periods
    • Consistent with the app's upgrade capability from version 1 to 2
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for upgrade-related configurations and height settings
rg -l "upgrade.*height|app.*version" --type-add 'proto:*.proto' --type proto --type go

# Check for any upgrade handlers that might need adjustment
ast-grep --pattern 'func ($x *Keeper) HandleUpgrade($y context.Context, $z Plan) {
  $$$
}'

Length of output: 1595


Script:

#!/bin/bash
# Let's check the specific upgrade height configuration in the script and related files
rg -A 5 "v2-upgrade-height" scripts/

# Check app version constants and initialization
rg -A 5 "InitialVersion|AppVersion" pkg/appconsts/

Length of output: 1162


Script:

#!/bin/bash
# Let's check the versioning approach in the codebase
rg -A 3 "version.*=.*1" pkg/appconsts/
rg -A 3 "version.*=.*1" app/

# Check for any upgrade height related constants or configurations
rg -A 3 "UpgradeHeight.*=|DefaultUpgradeHeight" 

Length of output: 4023

app/app.go (2)

462-462: LGTM: Added helpful debug logging.

The addition of debug logging for the current version is valuable for troubleshooting upgrade issues.


Line range hint 480-484: LGTM: Improved version upgrade logic.

The changes enhance the version upgrade process by:

  1. Adding informative logging for version transitions
  2. Ensuring version changes are strictly increasing
  3. Using SignalKeeper for coordinated upgrades

Comment on lines 18 to 24
CHAIN_ID="test"
KEY_NAME="validator"
KEYRING_BACKEND="test"
CELESTIA_APP_HOME="${HOME}/.celestia-app"
CELESTIA_APP_VERSION=$(celestia-appd version 2>&1)
FEES="500utia"
BROADCAST_MODE="block"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Make environment variables configurable

The script uses hardcoded values which could limit its reusability across different environments.

Consider making these values configurable:

-CHAIN_ID="test"
-KEY_NAME="validator"
-KEYRING_BACKEND="test"
-FEES="500utia"
+CHAIN_ID="${CHAIN_ID:-test}"
+KEY_NAME="${KEY_NAME:-validator}"
+KEYRING_BACKEND="${KEYRING_BACKEND:-test}"
+FEES="${FEES:-500utia}"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
CHAIN_ID="test"
KEY_NAME="validator"
KEYRING_BACKEND="test"
CELESTIA_APP_HOME="${HOME}/.celestia-app"
CELESTIA_APP_VERSION=$(celestia-appd version 2>&1)
FEES="500utia"
BROADCAST_MODE="block"
CHAIN_ID="${CHAIN_ID:-test}"
KEY_NAME="${KEY_NAME:-validator}"
KEYRING_BACKEND="${KEYRING_BACKEND:-test}"
CELESTIA_APP_HOME="${HOME}/.celestia-app"
CELESTIA_APP_VERSION=$(celestia-appd version 2>&1)
FEES="${FEES:-500utia}"
BROADCAST_MODE="block"

Comment on lines 54 to 55
echo "Querying for pending upgrade..."
celestia-appd query signal upgrade
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Verify upgrade status

The script queries for pending upgrade but doesn't verify the result or provide clear success/failure indication.

Add verification of the upgrade status:

 echo "Querying for pending upgrade..."
-celestia-appd query signal upgrade
+UPGRADE_STATUS=$(celestia-appd query signal upgrade -o json)
+if echo "$UPGRADE_STATUS" | jq -e '.upgrade.plan.height' > /dev/null; then
+    echo "Success: Upgrade to v3 is pending at height $(echo "$UPGRADE_STATUS" | jq -r '.upgrade.plan.height')"
+else
+    echo "Error: No pending upgrade found"
+    exit 1
+fi
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
echo "Querying for pending upgrade..."
celestia-appd query signal upgrade
echo "Querying for pending upgrade..."
UPGRADE_STATUS=$(celestia-appd query signal upgrade -o json)
if echo "$UPGRADE_STATUS" | jq -e '.upgrade.plan.height' > /dev/null; then
echo "Success: Upgrade to v3 is pending at height $(echo "$UPGRADE_STATUS" | jq -r '.upgrade.plan.height')"
else
echo "Error: No pending upgrade found"
exit 1
fi

Comment on lines 3 to 5
# Prerequisites: ensure ./single-node.sh is running in another terminal.
# Wait until block height is 3 for the node to upgrade from v1 -> v2.
# This script will upgrade the node from v2 -> v3.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Consider adding block height verification

The comment indicates waiting for block height 3 is required, but the script doesn't verify this condition. This could lead to upgrade failures if run too early.

Add block height verification before proceeding:

+# Verify current block height
+CURRENT_HEIGHT=$(celestia-appd query block | jq -r '.block.header.height')
+if [ "$CURRENT_HEIGHT" -lt "3" ]; then
+    echo "Error: Current block height ($CURRENT_HEIGHT) is less than required height (3)"
+    exit 1
+fi

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines 31 to 52
echo "Submitting signal for v3..."
celestia-appd tx signal signal 3 \
--keyring-backend=${KEYRING_BACKEND} \
--home ${CELESTIA_APP_HOME} \
--from ${KEY_NAME} \
--fees ${FEES} \
--chain-id ${CHAIN_ID} \
--broadcast-mode ${BROADCAST_MODE} \
--yes

echo "Querying the tally for v3..."
celestia-appd query signal tally 3

echo "Submitting msg try upgrade..."
celestia-appd tx signal try-upgrade \
--keyring-backend=${KEYRING_BACKEND} \
--home ${CELESTIA_APP_HOME} \
--from ${KEY_NAME} \
--fees ${FEES} \
--chain-id ${CHAIN_ID} \
--broadcast-mode ${BROADCAST_MODE} \
--yes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling between upgrade steps

The upgrade commands are executed sequentially without verifying the success of each step. This could lead to incomplete upgrades.

Consider adding verification between steps:

 echo "Submitting signal for v3..."
 celestia-appd tx signal signal 3 \
     --keyring-backend=${KEYRING_BACKEND} \
     --home ${CELESTIA_APP_HOME} \
     --from ${KEY_NAME} \
     --fees ${FEES} \
     --chain-id ${CHAIN_ID} \
     --broadcast-mode ${BROADCAST_MODE} \
     --yes
+
+# Verify signal submission success
+sleep 5  # Wait for transaction to be processed
+SIGNAL_COUNT=$(celestia-appd query signal tally 3 -o json | jq '.count')
+if [ "$SIGNAL_COUNT" -eq "0" ]; then
+    echo "Error: Signal submission failed"
+    exit 1
+fi

Committable suggestion skipped: line range outside the PR's diff.

rach-id
rach-id previously approved these changes Nov 20, 2024
app/app.go Outdated Show resolved Hide resolved
app/app.go Outdated Show resolved Hide resolved
scripts/upgrade-to-v3.sh Outdated Show resolved Hide resolved
@celestia-bot celestia-bot requested review from a team and staheri14 and removed request for a team November 20, 2024 21:28
rootulp and others added 2 commits November 20, 2024 16:28
Co-authored-by: CHAMI Rachid <[email protected]>
Co-authored-by: CHAMI Rachid <[email protected]>
@celestia-bot celestia-bot requested a review from a team November 20, 2024 21:28
@rootulp rootulp requested a review from rach-id November 20, 2024 21:28
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b6470c7 and 8e9dd1f.

📒 Files selected for processing (2)
  • app/app.go (2 hunks)
  • scripts/upgrade-to-v3.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • scripts/upgrade-to-v3.sh
🧰 Additional context used
🪛 GitHub Check: test / test-race
app/app.go

[failure] 480-480:
syntax error: unexpected ) at end of statement


[failure] 480-480:
syntax error: unexpected ) at end of statement


[failure] 480-480:
syntax error: unexpected ) at end of statement


[failure] 480-480:
syntax error: unexpected ) at end of statement


[failure] 480-480:
syntax error: unexpected ) at end of statement

🪛 GitHub Check: test / test-fuzz
app/app.go

[failure] 480-480:
syntax error: unexpected ) at end of statement

🪛 GitHub Check: test / test-short
app/app.go

[failure] 480-480:
syntax error: unexpected ) at end of statement


[failure] 480-480:
syntax error: unexpected ) at end of statement


[failure] 480-480:
syntax error: unexpected ) at end of statement


[failure] 480-480:
syntax error: unexpected ) at end of statement


[failure] 480-480:
syntax error: unexpected ) at end of statement

🪛 GitHub Check: test / test
app/app.go

[failure] 480-480:
syntax error: unexpected ) at end of statement


[failure] 480-480:
syntax error: unexpected ) at end of statement


[failure] 480-480:
syntax error: unexpected ) at end of statement


[failure] 480-480:
syntax error: unexpected ) at end of statement


[failure] 480-480:
syntax error: unexpected ) at end of statement

🪛 GitHub Check: lint / golangci-lint
app/app.go

[failure] 480-480:
syntax error: unexpected ) at end of statement) (typecheck)


[failure] 480-480:
syntax error: unexpected ) at end of statement) (typecheck)


[failure] 480-480:
syntax error: unexpected ) at end of statement) (typecheck)

app/app.go Outdated Show resolved Hide resolved
rootulp and others added 2 commits November 21, 2024 09:23
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
evan-forbes
evan-forbes previously approved these changes Nov 21, 2024
@rootulp rootulp requested review from ninabarbakadze and cmwaters and removed request for staheri14 November 22, 2024 14:03
rach-id
rach-id previously approved these changes Nov 25, 2024
@rootulp rootulp enabled auto-merge (squash) November 25, 2024 14:20
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would actually think it would be more convenient that the single-node.sh scripts starts from the latest version. We're no longer that concerned with v1 and soon to be v2

app/app.go Outdated Show resolved Hide resolved
@rootulp
Copy link
Collaborator Author

rootulp commented Nov 25, 2024

I would actually think it would be more convenient that the single-node.sh scripts starts from the latest version. We're no longer that concerned with v1 and soon to be v2

That is fine with me. I'll move the current single-node.sh stuff to a new script that actually starts the chain at v1, upgrades to v2, then to v3 so that we can easily test for app version edge cases.

Even though we only care about the v3 state machine for current modifications, I think it's still important to test that we can do v1 -> v2 -> v3 correctly b/c that's what mainnet nodes do.

@rootulp rootulp marked this pull request as draft November 25, 2024 14:31
auto-merge was automatically disabled November 25, 2024 14:31

Pull request was converted to draft

@rootulp rootulp dismissed stale reviews from rach-id and evan-forbes via 7d4558e November 25, 2024 16:00
@rootulp rootulp marked this pull request as ready for review November 25, 2024 21:10
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (3)
scripts/single-node-upgrades.sh (3)

11-15: Enhance error message for missing binary

The error message could be more specific about PATH verification. Consider adding instructions to verify the PATH setting.

-    echo "celestia-appd could not be found. Please install the celestia-appd binary using 'make install' and make sure the PATH contains the directory where the binary exists. By default, go will install the binary under '~/go/bin'"
+    echo "Error: celestia-appd not found in PATH"
+    echo "1. Install using: make install"
+    echo "2. Verify PATH includes: ~/go/bin"
+    echo "3. Verify installation: which celestia-appd"

24-31: Improve version check output formatting

The version check output could be more structured and include validation.

-VERSION=$(celestia-appd version 2>&1)
-
-echo "celestia-app version: ${VERSION}"
-echo "celestia-app home: ${APP_HOME}"
-echo "celestia-app genesis file: ${GENESIS_FILE}"
-echo ""
+VERSION=$(celestia-appd version 2>&1) || {
+    echo "Error: Failed to get celestia-appd version"
+    exit 1
+}
+
+echo "Configuration:"
+echo "=============="
+echo "Version:       ${VERSION}"
+echo "Home:         ${APP_HOME}"
+echo "Genesis:      ${GENESIS_FILE}"
+echo "=============="

110-110: Consider making upgrade height configurable

The hardcoded upgrade height might not be suitable for all scenarios.

+# At the top with other constants
+V2_UPGRADE_HEIGHT=${V2_UPGRADE_HEIGHT:-3}
+
-    --v2-upgrade-height 3 \
+    --v2-upgrade-height "${V2_UPGRADE_HEIGHT}" \
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 434f6ba and f7be64c.

📒 Files selected for processing (3)
  • app/app.go (1 hunks)
  • scripts/single-node-upgrades.sh (1 hunks)
  • scripts/single-node.sh (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • app/app.go
  • scripts/single-node.sh

Comment on lines +33 to +96
createGenesis() {
echo "Initializing validator and node config files..."
celestia-appd init ${CHAIN_ID} \
--chain-id ${CHAIN_ID} \
--home "${APP_HOME}" \
> /dev/null 2>&1 # Hide output to reduce terminal noise

echo "Adding a new key to the keyring..."
celestia-appd keys add ${KEY_NAME} \
--keyring-backend=${KEYRING_BACKEND} \
--home "${APP_HOME}" \
> /dev/null 2>&1 # Hide output to reduce terminal noise

echo "Adding genesis account..."
celestia-appd add-genesis-account \
"$(celestia-appd keys show ${KEY_NAME} -a --keyring-backend=${KEYRING_BACKEND} --home "${APP_HOME}")" \
"1000000000000000utia" \
--home "${APP_HOME}"

echo "Creating a genesis tx..."
celestia-appd gentx ${KEY_NAME} 5000000000utia \
--fees ${FEES} \
--keyring-backend=${KEYRING_BACKEND} \
--chain-id ${CHAIN_ID} \
--home "${APP_HOME}" \
> /dev/null 2>&1 # Hide output to reduce terminal noise

echo "Collecting genesis txs..."
celestia-appd collect-gentxs \
--home "${APP_HOME}" \
> /dev/null 2>&1 # Hide output to reduce terminal noise

# If you encounter: `sed: -I or -i may not be used with stdin` on MacOS you can mitigate by installing gnu-sed
# https://gist.github.com/andre3k1/e3a1a7133fded5de5a9ee99c87c6fa0d?permalink_comment_id=3082272#gistcomment-3082272

# Override the default RPC server listening address
sed -i'.bak' 's#"tcp://127.0.0.1:26657"#"tcp://0.0.0.0:26657"#g' "${APP_HOME}"/config/config.toml

# Enable transaction indexing
sed -i'.bak' 's#"null"#"kv"#g' "${APP_HOME}"/config/config.toml

# Persist ABCI responses
sed -i'.bak' 's#discard_abci_responses = true#discard_abci_responses = false#g' "${APP_HOME}"/config/config.toml

# Override the genesis to use app version 1 and then upgrade to app version 2 later.
sed -i'.bak' 's/"app_version": *"[^"]*"/"app_version": "1"/' ${APP_HOME}/config/genesis.json

# Override the log level to debug
# sed -i'.bak' 's#log_level = "info"#log_level = "debug"#g' "${APP_HOME}"/config/config.toml

# Override the VotingPeriod from 1 week to 1 minute
sed -i'.bak' 's#"604800s"#"60s"#g' "${APP_HOME}"/config/genesis.json

trace_type="local"
sed -i.bak -e "s/^trace_type *=.*/trace_type = \"$trace_type\"/" ${APP_HOME}/config/config.toml

trace_pull_address=":26661"
sed -i.bak -e "s/^trace_pull_address *=.*/trace_pull_address = \"$trace_pull_address\"/" ${APP_HOME}/config/config.toml

trace_push_batch_size=1000
sed -i.bak -e "s/^trace_push_batch_size *=.*/trace_push_batch_size = \"$trace_push_batch_size\"/" ${APP_HOME}/config/config.toml

echo "Tracing is set up with the ability to pull traced data from the node on the address http://127.0.0.1${trace_pull_address}"
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor createGenesis function for better maintainability

The function is handling too many responsibilities and could benefit from being split into smaller, focused functions.

Consider refactoring into these functions:

  1. initializeNode
  2. setupKeyring
  3. configureGenesis
  4. configureNetwork
  5. configureTracing

Example structure:

initializeNode() {
    echo "Initializing validator and node config files..."
    celestia-appd init ${CHAIN_ID} \
        --chain-id ${CHAIN_ID} \
        --home "${APP_HOME}" \
        > /dev/null 2>&1
}

setupKeyring() {
    echo "Adding a new key to the keyring..."
    celestia-appd keys add ${KEY_NAME} \
        --keyring-backend=${KEYRING_BACKEND} \
        --home "${APP_HOME}" \
        > /dev/null 2>&1
    # ... rest of keyring setup
}

createGenesis() {
    initializeNode
    setupKeyring
    configureGenesis
    configureNetwork
    configureTracing
}

Comment on lines +98 to +101
deleteCelestiaAppHome() {
echo "Deleting $APP_HOME..."
rm -r "$APP_HOME"
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add safety checks to deleteCelestiaAppHome function

The function should verify the path and ask for confirmation before deletion.

 deleteCelestiaAppHome() {
+    if [ -z "${APP_HOME}" ] || [ "${APP_HOME}" = "/" ]; then
+        echo "Error: Invalid APP_HOME path"
+        return 1
+    fi
+
+    if [ ! -d "${APP_HOME}" ]; then
+        echo "Nothing to delete: ${APP_HOME} does not exist"
+        return 0
+    fi
+
     echo "Deleting $APP_HOME..."
     rm -r "$APP_HOME"
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
deleteCelestiaAppHome() {
echo "Deleting $APP_HOME..."
rm -r "$APP_HOME"
}
deleteCelestiaAppHome() {
if [ -z "${APP_HOME}" ] || [ "${APP_HOME}" = "/" ]; then
echo "Error: Invalid APP_HOME path"
return 1
fi
if [ ! -d "${APP_HOME}" ]; then
echo "Nothing to delete: ${APP_HOME} does not exist"
return 0
fi
echo "Deleting $APP_HOME..."
rm -r "$APP_HOME"
}

Comment on lines +68 to +79
# Override the default RPC server listening address
sed -i'.bak' 's#"tcp://127.0.0.1:26657"#"tcp://0.0.0.0:26657"#g' "${APP_HOME}"/config/config.toml

# Enable transaction indexing
sed -i'.bak' 's#"null"#"kv"#g' "${APP_HOME}"/config/config.toml

# Persist ABCI responses
sed -i'.bak' 's#discard_abci_responses = true#discard_abci_responses = false#g' "${APP_HOME}"/config/config.toml

# Override the genesis to use app version 1 and then upgrade to app version 2 later.
sed -i'.bak' 's/"app_version": *"[^"]*"/"app_version": "1"/' ${APP_HOME}/config/genesis.json

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling for sed commands

The configuration modifications using sed lack error handling and could fail silently.

+modify_config() {
+    local file="$1"
+    local search="$2"
+    local replace="$3"
+    local backup="${file}.bak"
+    
+    if ! sed -i.bak "s#${search}#${replace}#g" "${file}"; then
+        echo "Error: Failed to modify ${file}"
+        return 1
+    fi
+    rm -f "${backup}"
+}

-sed -i'.bak' 's#"tcp://127.0.0.1:26657"#"tcp://0.0.0.0:26657"#g' "${APP_HOME}"/config/config.toml
+modify_config "${APP_HOME}/config/config.toml" \
+    '"tcp://127.0.0.1:26657"' \
+    '"tcp://0.0.0.0:26657"'

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +145 to +157
if [ -f $GENESIS_FILE ]; then
echo "Do you want to delete existing ${APP_HOME} and start a new local testnet? [y/n]"
read -r response
if [ "$response" = "y" ]; then
deleteCelestiaAppHome
createGenesis
fi
else
createGenesis
fi

upgradeToV3 & # Start the upgrade process from v2 -> v3 in the background.
startCelestiaApp # Start celestia-app in the foreground.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve main execution flow reliability

The main execution flow needs better process management and cleanup.

+cleanup() {
+    local pids=$(jobs -p)
+    if [ -n "${pids}" ]; then
+        kill ${pids} 2>/dev/null
+    fi
+}
+
+trap cleanup EXIT
+
 if [ -f $GENESIS_FILE ]; then
   echo "Do you want to delete existing ${APP_HOME} and start a new local testnet? [y/n]"
   read -r response
   if [ "$response" = "y" ]; then
     deleteCelestiaAppHome
     createGenesis
   fi
 else
   createGenesis
 fi

-upgradeToV3 & # Start the upgrade process from v2 -> v3 in the background.
-startCelestiaApp # Start celestia-app in the foreground.
+upgradeToV3 & 
+UPGRADE_PID=$!
+
+startCelestiaApp &
+APP_PID=$!
+
+wait $APP_PID || {
+    echo "Error: celestia-app process failed"
+    exit 1
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if [ -f $GENESIS_FILE ]; then
echo "Do you want to delete existing ${APP_HOME} and start a new local testnet? [y/n]"
read -r response
if [ "$response" = "y" ]; then
deleteCelestiaAppHome
createGenesis
fi
else
createGenesis
fi
upgradeToV3 & # Start the upgrade process from v2 -> v3 in the background.
startCelestiaApp # Start celestia-app in the foreground.
cleanup() {
local pids=$(jobs -p)
if [ -n "${pids}" ]; then
kill ${pids} 2>/dev/null
fi
}
trap cleanup EXIT
if [ -f $GENESIS_FILE ]; then
echo "Do you want to delete existing ${APP_HOME} and start a new local testnet? [y/n]"
read -r response
if [ "$response" = "y" ]; then
deleteCelestiaAppHome
createGenesis
fi
else
createGenesis
fi
upgradeToV3 &
UPGRADE_PID=$!
startCelestiaApp &
APP_PID=$!
wait $APP_PID || {
echo "Error: celestia-app process failed"
exit 1
}

Comment on lines +114 to +143
upgradeToV3() {
sleep 45
echo "Submitting signal for v3..."
celestia-appd tx signal signal 3 \
--keyring-backend=${KEYRING_BACKEND} \
--home ${APP_HOME} \
--from ${KEY_NAME} \
--fees ${FEES} \
--chain-id ${CHAIN_ID} \
--broadcast-mode ${BROADCAST_MODE} \
--yes \
> /dev/null 2>&1 # Hide output to reduce terminal noise

echo "Querying the tally for v3..."
celestia-appd query signal tally 3

echo "Submitting msg try upgrade..."
celestia-appd tx signal try-upgrade \
--keyring-backend=${KEYRING_BACKEND} \
--home ${APP_HOME} \
--from ${KEY_NAME} \
--fees ${FEES} \
--chain-id ${CHAIN_ID} \
--broadcast-mode ${BROADCAST_MODE} \
--yes \
> /dev/null 2>&1 # Hide output to reduce terminal noise

echo "Querying for pending upgrade..."
celestia-appd query signal upgrade
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve upgrade process reliability

The upgrade process has several areas that could be improved for better reliability:

  1. The hardcoded sleep duration
  2. Lack of upgrade verification
  3. Missing error handling
 upgradeToV3() {
-    sleep 45
+    local max_attempts=30
+    local attempt=1
+    local sleep_duration=10
+
+    echo "Waiting for chain to reach v2 upgrade height..."
+    while [ $attempt -le $max_attempts ]; do
+        if celestia-appd query block 2>/dev/null | grep -q '"version": "2"'; then
+            echo "Chain successfully upgraded to v2"
+            break
+        fi
+        echo "Attempt $attempt/$max_attempts: Chain not yet at v2..."
+        sleep $sleep_duration
+        attempt=$((attempt + 1))
+    done
+
+    if [ $attempt -gt $max_attempts ]; then
+        echo "Error: Timeout waiting for v2 upgrade"
+        return 1
+    fi

     echo "Submitting signal for v3..."
-    celestia-appd tx signal signal 3 \
+    if ! celestia-appd tx signal signal 3 \
         --keyring-backend=${KEYRING_BACKEND} \
         --home ${APP_HOME} \
         --from ${KEY_NAME} \
         --fees ${FEES} \
         --chain-id ${CHAIN_ID} \
         --broadcast-mode ${BROADCAST_MODE} \
-        --yes \
-        > /dev/null 2>&1
+        --yes > /dev/null 2>&1; then
+        echo "Error: Failed to submit v3 upgrade signal"
+        return 1
+    fi
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
upgradeToV3() {
sleep 45
echo "Submitting signal for v3..."
celestia-appd tx signal signal 3 \
--keyring-backend=${KEYRING_BACKEND} \
--home ${APP_HOME} \
--from ${KEY_NAME} \
--fees ${FEES} \
--chain-id ${CHAIN_ID} \
--broadcast-mode ${BROADCAST_MODE} \
--yes \
> /dev/null 2>&1 # Hide output to reduce terminal noise
echo "Querying the tally for v3..."
celestia-appd query signal tally 3
echo "Submitting msg try upgrade..."
celestia-appd tx signal try-upgrade \
--keyring-backend=${KEYRING_BACKEND} \
--home ${APP_HOME} \
--from ${KEY_NAME} \
--fees ${FEES} \
--chain-id ${CHAIN_ID} \
--broadcast-mode ${BROADCAST_MODE} \
--yes \
> /dev/null 2>&1 # Hide output to reduce terminal noise
echo "Querying for pending upgrade..."
celestia-appd query signal upgrade
}
upgradeToV3() {
local max_attempts=30
local attempt=1
local sleep_duration=10
echo "Waiting for chain to reach v2 upgrade height..."
while [ $attempt -le $max_attempts ]; do
if celestia-appd query block 2>/dev/null | grep -q '"version": "2"'; then
echo "Chain successfully upgraded to v2"
break
fi
echo "Attempt $attempt/$max_attempts: Chain not yet at v2..."
sleep $sleep_duration
attempt=$((attempt + 1))
done
if [ $attempt -gt $max_attempts ]; then
echo "Error: Timeout waiting for v2 upgrade"
return 1
fi
echo "Submitting signal for v3..."
if ! celestia-appd tx signal signal 3 \
--keyring-backend=${KEYRING_BACKEND} \
--home ${APP_HOME} \
--from ${KEY_NAME} \
--fees ${FEES} \
--chain-id ${CHAIN_ID} \
--broadcast-mode ${BROADCAST_MODE} \
--yes > /dev/null 2>&1; then
echo "Error: Failed to submit v3 upgrade signal"
return 1
fi
echo "Querying the tally for v3..."
celestia-appd query signal tally 3
echo "Submitting msg try upgrade..."
celestia-appd tx signal try-upgrade \
--keyring-backend=${KEYRING_BACKEND} \
--home ${APP_HOME} \
--from ${KEY_NAME} \
--fees ${FEES} \
--chain-id ${CHAIN_ID} \
--broadcast-mode ${BROADCAST_MODE} \
--yes \
> /dev/null 2>&1 # Hide output to reduce terminal noise
echo "Querying for pending upgrade..."
celestia-appd query signal upgrade
}

@rootulp rootulp changed the title fix: improve single-node.sh chore: improve single-node.sh Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

celestia-appd status reports app version 3 prematurely
4 participants